Skip to content

perf: probing access without exception, cache MethodHandles #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 9, 2024

Conversation

jbee
Copy link
Collaborator

@jbee jbee commented Aug 9, 2024

Background

I ran generating OpenAPI HTML with profiling on. Interestingly the most expensive methods had to do with method handle resolve and filling in stack trace information of thrown exceptions. It was clear that something that happens very often must throw exceptions and that the method handle resolving was either slow itself or also did throw exceptions.

Reasoning

I knew that the JsonValue API does some of its evaluations by trying to access a node and then catch the potential exception in case the node did not exist. This was an obvious candidate for being responsible for countless exceptions. So I created alternative methods that would not throw an exception but return null instead.

Secondly I added caching to the method handle lookup. I also found when trying my improvements in the profiler that the method handle resolution itself was one source of throwing and then catching an exception. So caching that avoided doing throw + catch a lot.

before

This image shows that vast amount of time was spend in fillInStackTrace and MethodHandleNatives.resolve and MethodHandleNatives.init. After these changes those rank far lower with times in the hundredths of ms not thousands (of the total time it took to generate the OpenAPI HTML)

after

Based on the methods highlighted by the profiler I also had a look at the skip-methods and simplified them a bit to allow better in-lining in case that was a reason for them ranking high and if looking at the numbers it did indeed improve noticeably.

Summary

  • feat: adds getOrNull, elementOrNull and memberOrNull methods to JsonNode to avoid exceptions when checking
  • perf: remember size of objects and arrays at the end of iterating their children
  • perf: adds early exit path for child lookup - if a child is not in the cache but size is known we can infer that all children are cached so the child we are looking for does not exist
  • perf: size() first uses size field if availanle
  • perf: MethodHandles are now cached (avoid internal exceptions)
  • perf: handling of paths as String which checks for a handful of special cases now compares a char instead of a String
  • perf: when invoking a method handle with no arguments invoke is used instead of invokeWithArguments (which does much more adjustment work that is not needed without arguments)
  • perf: avoid lambdas in skipWhitespace as it is used a lot

Automatic Testing

I added some basic test for getOrNull. This internally uses elementOrNull and memberOrNull. The new test scenarios only test the "not existing" case since the positive case is tested plenty indirectly by lots of other tests as the JsonValue implementation now mostly uses getOrNull. But some methods sill use get as they want exceptions so both essential methods are still well tested.

JDK

As far as I can tell the exception thrown as part of resolving a MethodHandle is in MemberName#resolve https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/invoke/MemberName.java#L957 which is caught https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/invoke/MemberName.java#L963
The exact exception thrown is a IncompatibleClassChangeError with the message "Found class java.lang.Object, but interface was expected". But when putting a breakpoint at the error constructor multiple other cases for invoke static method handles also occur which never escape the method handle lookup itself (code has the intended result). I would assume that all these cases not necessarily have to cause throw + catch and could equally be tested for using a if with the equivalent condition somewhere on the outside.

@jbee jbee self-assigned this Aug 9, 2024
@jbee jbee changed the title perf: access without exception, cache MethodHandles perf: probing access without exception, cache MethodHandles Aug 9, 2024
@jbee jbee marked this pull request as ready for review August 9, 2024 10:43
Copy link

@david-mackessy david-mackessy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding perf improvement is always nice, good stuff 👍
The getOrNull methods that only exist to throw exceptions (never returning their return type) don't make sense to someone who doesn't know anything about this library :)

@jbee
Copy link
Collaborator Author

jbee commented Aug 9, 2024

The getOrNull methods that only exist to throw exceptions (never returning their return type) don't make sense to someone who doesn't know anything about this library :)

Oh it does return null where get would throw the JsonPathException. Or maybe I misunderstand what you mean.

@david-mackessy
Copy link

The getOrNull methods that only exist to throw exceptions (never returning their return type) don't make sense to someone who doesn't know anything about this library :)

Oh it does return null where get would throw the JsonPathException. Or maybe I misunderstand what you mean.

Like this method:

@Maybe
default JsonNode getOrNull(@Surly JsonPath path) throws JsonPathException {
    throw new JsonPathException( path,
        format( "This is a leaf node of type %s that does not have any children at path: %s", getType(), path ) );
}

It looks like it will always throw an exception and never return a JsonNode. Does this result in null being returned somehow?

@jbee
Copy link
Collaborator Author

jbee commented Aug 9, 2024

Ah! yes. It might still be uncommon to use default methods this much but this is the same old pattern that people have used with abstract classes where those subtypes that implement a method do override it. I am sure this will become more and more common practice once more projects discover how neat defaults methods are :D

@david-mackessy
Copy link

Ah! yes. It might still be uncommon to use default methods this much but this is the same old pattern that in people have used with abstract classes where those subtypes that implement a method do override it. I am sure this will become more and more common practice once more projects discover how neat defaults methods are :D

Ah, ok. Looking at the code in a PR (not in IDE) makes it easy to miss things like this :)

@jbee jbee merged commit 9fe29dc into dhis2:main Aug 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants